Skip to content

feat: mark ukrainian links differently#11

Open
undead404 wants to merge 3 commits intomainfrom
feat/mark-ukrainian-links
Open

feat: mark ukrainian links differently#11
undead404 wants to merge 3 commits intomainfrom
feat/mark-ukrainian-links

Conversation

@undead404
Copy link
Contributor

@undead404 undead404 commented Mar 11, 2023

Mark Ukrainian & other links with some distinct class names.

@undead404 undead404 added the enhancement New feature or request label Mar 11, 2023
@undead404 undead404 requested a review from AdriandeCita March 11, 2023 05:57
@undead404 undead404 self-assigned this Mar 11, 2023
@undead404 undead404 mentioned this pull request Mar 11, 2023
Open
AdriandeCita
AdriandeCita reviewed Mar 11, 2023
View reviewed changes
AdriandeCita
AdriandeCita reviewed Mar 11, 2023
View reviewed changes
AdriandeCita
AdriandeCita requested changes Mar 11, 2023
View reviewed changes
Copy link
Member

@AdriandeCita AdriandeCita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Дякую, прикольна фіча. Я досі не певен в її практичності, але як ідея — гарно.

Пара зауважень стосовно класів.

  • ми можемо зробити цю логіку більш загальною? У нас насправді частка uk виникає не зі стелі, а з параметрів раннера
    image

Код процесора (принаймні, теоретично), можна швидко адаптувати під будь-яку локаль перекладу. А функція isUrlUkrainian порушує цей принцип. Тоді сам клас wd-link__ukrainian краще буде представити якось на кшталт wd-link__target-locale, а вже платформа буде вирішувати, який прапорець (чи не прапорець) там показувати.
Регулярки для урлів теж краще передавати зовні, щоб на кожну нову урлу не доводилося випускати нову версію бібліотеки.

@undead404
Copy link
Contributor Author

undead404 commented Mar 11, 2023

Хіба що перейменувати isUrlUkrainian на isUrlInLocalLanguage. Код цієї функції ж треба буде на кожну мову писати окремо.

@undead404
Copy link
Contributor Author

undead404 commented Mar 11, 2023

Тоді можна буде переписати тіло цієї функції – і контент-процесор буде готовий

@AdriandeCita
Copy link
Member

AdriandeCita commented Mar 11, 2023

чому окремо, якщо ти просто використовуєш масив регулярок? Тим паче, якщо їх передавати зовні?
Одна функція, просто перебирання передані регулярки - і все? Чи я тут упускаю якісь підводні граблі?)

@undead404
Copy link
Contributor Author

undead404 commented Mar 11, 2023

Передавати зовні регулярки? Звідки? Щось ти дуже ускладнюєш

@undead404 @AdriandeCita
Apply suggestions from code review
5a4f9e2 
Co-authored-by: Mykola Myslovskyi <adriandecita@gmail.com>
@AdriandeCita
Copy link
Member

AdriandeCita commented Mar 11, 2023

ага, карту редиректів ми передаємо, а масив регулярок передати не можемо?)

@AdriandeCita
Copy link
Member

AdriandeCita commented Mar 11, 2023 •
edited
Loading

ох, дідько, нафіга я той ПР змержив, якщо я передаю редиректи зовні... По ходу треба буде його сьогодні ревертнути, поки воно в СІ не потрапило х)
Поправив

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AdriandeCita AdriandeCita AdriandeCita requested changes

Assignees

@undead404 undead404

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@undead404 @AdriandeCita @viperehonchuk

Comments